refactor: simplify user parsing#571
Conversation
📝 WalkthroughWalkthroughThis PR converts configuration fields from comma-delimited strings to slices of strings (TrustedProxies, Users, OAuth whitelist), updating parsing utilities and all downstream usages throughout the codebase to operate on typed arrays instead of string-splitting operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #571 +/- ##
==========================================
- Coverage 19.23% 19.12% -0.11%
==========================================
Files 39 39
Lines 2298 2295 -3
==========================================
- Hits 442 439 -3
Misses 1828 1828
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/config/config.go (1)
39-39: Consider updating field descriptions to reflect slice types.The field description still mentions "Comma-separated list" while the type is now
[]string. If the configuration loader parses CSV input into slices automatically, this description is fine. Otherwise, consider updating it to reflect that it's a list/array type.internal/service/auth_service.go (1)
30-30: Consider refactoring to avoid join/split cycle.The
OauthWhitelistfield correctly changed to[]string. However, inIsEmailWhitelisted(line 190), the slice is joined into a comma-separated string before passing toCheckFilter, which then splits it back into a slice (as seen in theCheckFilterimplementation from internal/utils/security_utils.go).While this preserves existing behavior correctly, consider refactoring
CheckFilterto accept[]stringdirectly to avoid the unnecessary join/split cycle.♻️ Potential optimization
Update
CheckFiltersignature to accept a slice parameter:func CheckFilter(filters []string, str string) bool {Then update the call site:
func (auth *AuthService) IsEmailWhitelisted(email string) bool { - return utils.CheckFilter(strings.Join(auth.config.OauthWhitelist, ","), email) + return utils.CheckFilter(auth.config.OauthWhitelist, email) }Note: This would require updating all
CheckFiltercall sites throughout the codebase.Also applies to: 190-190
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/bootstrap/router_bootstrap.gointernal/config/config.gointernal/controller/proxy_controller_test.gointernal/controller/user_controller_test.gointernal/service/auth_service.gointernal/utils/user_utils.gointernal/utils/user_utils_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T14:57:03.955Z
Learnt from: steveiliop56
Repo: steveiliop56/tinyauth PR: 211
File: internal/utils/utils.go:373-407
Timestamp: 2025-06-25T14:57:03.955Z
Learning: In Go applications using the gin web framework, IP addresses extracted from gin.Context (like from request headers or remote address) are already validated by the framework and don't require additional validation in utility functions that process them.
Applied to files:
internal/bootstrap/router_bootstrap.go
🧬 Code graph analysis (3)
internal/utils/user_utils_test.go (1)
internal/utils/user_utils.go (2)
GetUsers(31-61)ParseUsers(10-29)
internal/service/auth_service.go (1)
internal/utils/security_utils.go (1)
CheckFilter(77-102)
internal/utils/user_utils.go (2)
internal/config/config.go (1)
User(119-123)internal/utils/fs_utils.go (1)
ReadFile(5-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
internal/controller/proxy_controller_test.go (1)
60-60: LGTM!The test correctly initializes
OauthWhitelistas an empty slice to match the updated type signature.internal/controller/user_controller_test.go (1)
63-63: LGTM!The test correctly initializes
OauthWhitelistas an empty slice to match the updated type signature.internal/bootstrap/router_bootstrap.go (1)
16-22: API compatibility verified — no issues found.The code correctly passes
app.config.Server.TrustedProxies(type[]string) toengine.SetTrustedProxies(), which matches gin v1.11.0's method signature. The length check before calling SetTrustedProxies is appropriate, and error handling is in place.internal/utils/user_utils.go (3)
10-29: LGTM!The
ParseUsersfunction correctly handles the slice-based input, properly trims whitespace, skips empty entries, and delegates toParseUserfor individual parsing. Error handling is appropriate.
63-92: Consider potential issue with$$replacement in bcrypt hashes.The
$$to$replacement on lines 64-66 could unintentionally modify bcrypt password hashes (which contain$as field separators like$2a$10$...). If a bcrypt hash happens to contain$$(unlikely but possible in the salt portion), it would be corrupted.This appears to be intentional escape handling (test on line 126 confirms
pa$$word123→pa$word123), but the placement before splitting means it affects the entire string including username and TOTP secret. Consider documenting this behavior or applying the replacement only to the password field after splitting.
31-61: No action needed. The project'sgo.modexplicitly specifies Go 1.24.0, andstrings.SplitSeqwas introduced in Go 1.24.0, so the code is fully compatible with the project's Go version requirement.internal/utils/user_utils_test.go (3)
12-75: LGTM!The
TestGetUsersfunction provides comprehensive coverage for the refactored API:
- File-only input (line 25)
- Config slice-only input (line 37)
- Both combined (line 49)
- Empty inputs (line 63)
- Non-existent file error handling (line 70)
All test cases correctly use the new
[]stringparameter type.
77-104: LGTM!The
TestParseUsersfunction correctly tests the slice-based API, including proper handling of whitespace-padded entries and TOTP secrets.
106-164: LGTM!The
TestParseUserfunction provides thorough coverage including:
- Users with and without TOTP
- The
$$escape sequence handling- Whitespace trimming
- Various invalid format scenarios
Good edge case coverage for error conditions.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.